Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add finalize subroutine for stochastic physics #32

Merged
merged 2 commits into from
Nov 28, 2020
Merged

add finalize subroutine for stochastic physics #32

merged 2 commits into from
Nov 28, 2020

Conversation

pjpegion
Copy link
Collaborator

This a new feature to add a finalize subroutine to deallocate array at the end of a run. Associated with PRs
NOAA-EMC/fv3atm#199 and
ufs-community/ufs-weather-model#278

@pjpegion pjpegion mentioned this pull request Nov 16, 2020
Merged
lisa-bengtsson
lisa-bengtsson approved these changes Nov 20, 2020
View reviewed changes
Copy link
Collaborator

@lisa-bengtsson lisa-bengtsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@HenryRWinterbottom
Copy link

I would strongly suggest adding if-def type statements to this PR. For example, instead of:

deallocate(variable)

do:

if(allocated(variable)) deallocate(variable)

This better protects the code from possible and difficult to chase segmentation faults in the future -- at least based on my experience.

@pjpegion
Copy link
Collaborator Author

@HenryWinterbottom-NOAA good point, I thought about doing it, but was just lazy.

@HenryRWinterbottom
Copy link

@pjpegion If you are planning to do it, just send me a note (or submit a new PR) and I am happy to approve.

@pjpegion
Copy link
Collaborator Author

@HenryWinterbottom-NOAA I added the if (allocated()) conditionals

HenryRWinterbottom
HenryRWinterbottom approved these changes Nov 20, 2020
View reviewed changes
Copy link

@HenryRWinterbottom HenryRWinterbottom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @pjpegion.

Approved.

climbfuji
climbfuji reviewed Nov 26, 2020
View reviewed changes
Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks okay; there are several places where a comment line

! Copy blocked data into contiguous arrays; no need to copy weights in (intent(out))

was moved around to places where it doesn't belong. I recommend removing or changing them, but certainly not a show stopper.

I am currently creating new baselines on cheyenne.intel, cheyenne.gnu and gaea.intel. The new baseline date tag will be 20201127, please update RTPWD in rt.sh accordingly.

@pjpegion
Copy link
Collaborator Author

Looks like the changes you are suggesting are for the fv3atm PR.

@climbfuji
Copy link
Collaborator

Looks like the changes you are suggesting are for the fv3atm PR.

Yes, sorry.

@pjpegion pjpegion merged commit e4913c0 into NOAA-PSL:master Nov 28, 2020
@pjpegion pjpegion deleted the stoch_fix branch July 30, 2021 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@climbfuji climbfuji climbfuji approved these changes

@HenryRWinterbottom HenryRWinterbottom HenryRWinterbottom approved these changes

@lisa-bengtsson lisa-bengtsson lisa-bengtsson approved these changes

@bingfu-NOAA bingfu-NOAA Awaiting requested review from bingfu-NOAA

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pjpegion @HenryRWinterbottom @climbfuji @lisa-bengtsson